Skip to content

Updated references to new Session() #3769

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 9 commits into from
Closed

Updated references to new Session() #3769

wants to merge 9 commits into from

Conversation

scottwarren
Copy link

If you use new Session(), you get a 500 error, with the following message:

Failed to start the session: already started by PHP.
Q A
Doc fix? yes
New docs? no
Applies to I tested 2.4.2, but it most likely will be earlier version as well
Fixed tickets none

scottwarren added 2 commits April 7, 2014 19:46
If you use new Session(), you get a 500 error, with the following message:

```log
Failed to start the session: already started by PHP.
```
As it is no longer needed
@wouterj
Copy link
Member

wouterj commented Apr 7, 2014

This article is about using the Sessions outside of the Symfony2 framework. It shouldn't cause any problems then. So I'm -1 for this change.

However, we might need to add a .. caution:: box saying that initializing the Session object causes the session to be started, which can lead to unexpected errors.

@scottwarren
Copy link
Author

Sorry about that - I misunderstood the context of the page. Thanks for looking at this, I have removed my changes and added a caution box instead :-)

.. caution::

Make sure your Sessions aren't already initliazed (which in Symfony is done automatically),
as initializing the Session object causes the session to be started, which can lead to unexpected errors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should break after the first word that crosses the 72th character.

@wouterj
Copy link
Member

wouterj commented Apr 7, 2014

Thank you for your quick response @scottwarren ! I've added 2 little comments, once you fixed those I'm very happy and @weaverryan will merge your PR :)

Added new lines after 72 characters to conform with character limit standards
@scottwarren
Copy link
Author

No worries, all fixed up now :-) - thanks for your help

@@ -12,6 +12,12 @@ object-oriented interface using a variety of session storage drivers.
Sessions are used via the simple :class:`Symfony\\Component\\HttpFoundation\\Session\\Session`
implementation of :class:`Symfony\\Component\\HttpFoundation\\Session\\SessionInterface` interface.

.. caution::

Make sure your Sessions aren't already initliazed (which in Symfony is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, small typo: initliazed -> initialized

And "which in Symfony is done automatically" should be "which is done automatically in the full-stack framework"

@scottwarren
Copy link
Author

Oops - sorry 👍 fixed

@@ -12,6 +12,13 @@ object-oriented interface using a variety of session storage drivers.
Sessions are used via the simple :class:`Symfony\\Component\\HttpFoundation\\Session\\Session`
implementation of :class:`Symfony\\Component\\HttpFoundation\\Session\\SessionInterface` interface.

.. caution::

Make sure your Sessions aren't already initialized (which is done
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sessions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which is, for example, automatically done in the full-stack framework

@scottwarren
Copy link
Author

Fixed

Make sure your sessions aren't already initialized (which is,
for example, automatically done in the full-stack framework).
As initializing the Session object causes the session to be
started, which can lead to unexpected errors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed: The text must be indented by four instead of three tabs. And you should use a length of 72 characters per line.

@scottwarren
Copy link
Author

Fixed

Make sure your sessions aren't already initialized (which is, for example,
automatically done in the full-stack framework). As initializing the
Session object causes the session to be started, which can lead to unexpected
errors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the comment about the full-stack framework adding anything here? I think it might be confusing what we're saying. As I understand it, the important message here is "make sure the PHP session hasn't already been started in some other way before using the Session object, else you'll get an error". Can we remove the full-stack framework comment to add clarity?

And if I'm understanding the message correctly, I think we can say it in one really quick sentence:

Make sure your PHP session isn't already started before using the Session class. If you have a legacy session system that starts your session, see [link to http://symfony.com/doc/current/components/http_foundation/session_php_bridge.html]

@scottwarren
Copy link
Author

Thanks @weaverryan I have updated the message

@weaverryan
Copy link
Member

Awesome - thanks Scott!

weaverryan added a commit that referenced this pull request May 12, 2014
This PR was submitted for the 2.4 branch but it was merged into the 2.3 branch instead (closes #3769).

Discussion
----------

Updated references to new Session()

If you use new Session(), you get a 500 error, with the following message:

```log
Failed to start the session: already started by PHP.
```

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | I tested 2.4.2, but it most likely will be earlier version as well
| Fixed tickets | none

Commits
-------

fa86275 Updated warning for Sessions
4e06f0e fixed spaces and 72 char line limit
3624fec Updated grammer issues in caution section
b2ac5bc fixed typo
6e6b19f Fixed typo and removed extra space
df4ef14 Added extra new lines
275dd97 Added Caution note
a64f274 Removed use command
490efdc Updated references to new Session()
@weaverryan weaverryan closed this May 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants